-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change read:me
scope to profile
scope
#30357
base: main
Are you sure you want to change the base?
Change read:me
scope to profile
scope
#30357
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it affect existing registrations with read:me
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file needs normalizing.
There shouldn't be any yet, given it's only in bleeding-edge We could always do a migration on oauth_applications and oauth_tokens to rewrite |
It's been merged roughly a month ago, and two weeks ago you made it the default for new apps created through the settings interface. Looking into mastodon.social's database, there are 4 applications with |
read:me
scope to profile
scope
Oh! Good point! I'll add database migrations to this PR then; basically a find scopes LIKE %read:me% and then each in batches to rewrite? |
PROFILE_TERM = 'profile' | ||
DEFAULT_TERM = 'all' | ||
DEFAULT_ACCESS = %w(read write).freeze | ||
READ_ONLY_ACCESS = %w(read).freeze | ||
|
||
attr_reader :namespace, :term | ||
|
||
def initialize(scope) | ||
@namespace = scope[:namespace]&.to_s | ||
@access = scope[:access] ? [scope[:access].to_s] : DEFAULT_ACCESS.dup | ||
@term = scope[:term]&.to_s || DEFAULT_TERM | ||
|
||
# override for profile scope which is read only | ||
@access = if @term == PROFILE_TERM | ||
READ_ONLY_ACCESS.dup | ||
else | ||
scope[:access] ? [scope[:access].to_s] : DEFAULT_ACCESS.dup | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may just be more readable as something like:
# override for profile scope which is read-only
@access = %w(read) if @term == 'profile'
The overhead of doing the assignment twice is negligible, and I think that special logic being self-contained in a single line is more readable than involving multiple constants.
This would resolve #30355, though introduced a quirk with the scope parser logic, which I'm not sure I've solved in an acceptable manner?
Basically the
profile
scope would otherwise default toall
which isread and write
, but forprofile
it will only ever beread
.